feat(network)!: add bounded service sessions and request resources - #943
Conversation
And one more auto-invalidated finding. Analyzed 10 files, diff |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9549ca2c86
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let opening_stream_count: usize = ordered_streams | ||
| .iter() | ||
| .map(|stream| self.registry.ordered_stream_pair(*stream).map_or(1, |_| 2)) | ||
| .sum(); |
There was a problem hiding this comment.
Exclude retired roles from the queue split
When a peer negotiates the two-role pair plus a service whose demand is Retire, and advertises max_inbound_queue_depth = 2, this active-role count is 2 but the following queue guard still compares the limit with negotiated_ordered_streams.len() (3), closing the connection before the usable pair starts. Fresh evidence in this revision is that opening_stream_count now excludes retired sessions for the stream-limit check, while queue validation and queue_split_stream_count still include them; derive those from the non-retired role set as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This also happens on main, so we'll handle excluding retired services from the queue split in a separate follow-up.
This comment was marked as resolved.
This comment was marked as resolved.
d908fe7 to
a5b40f2
Compare
|
forwarding ai audit |
refactor(network): generalize paired streams into service sessions
596f91d to
a79e041
Compare
Main's #943 added Service::message_payload_limits. The transport checks those limits from the frame header before it allocates a payload, which is the job this PR's MessagePayloadCap did. The two mechanisms conflicted in handler.rs, transport/service.rs, transport/mod.rs, and block_sync/service.rs. Take main's mechanism and drop this PR's copy: the MessagePayloadCap type, the Stream::message_payload_caps field, the read_frame parameter, the registry cap validation, and their tests. Main's message_payload_limits_apply_before_payload_reads covers the same header-time rejection. Keep this PR's GetBlocks limit by declaring it through the new hook: BlockSyncService::message_payload_limits returns (MSG_BS_GET_BLOCKS, GET_BLOCKS.payload_cap) for the block-sync stream. The declaration test now asserts that declared limit.
handler.rs conflicted in three places: - The imports take main's list, add this PR's control-handshake items, and drop the inline-handshake items that control_handshake.rs now owns. - The helper block keeps main's new read_frame_payload and #[cfg(test)] write_ordered_frame. It drops read_control_payload and write_control_payload, which this PR moved into control_handshake.rs. Main did not change either helper. Main's #943 and #968 changed types that the handshake testkit uses: - scenario.rs sets the new ZakuraLocalLimits::nat_traversal field to false, the production default. - runner.rs classifies the new SessionFull error as a local resource rejection, and InvalidMessageType and InvalidServiceSession as peer violations.
Motivation
A blocked P2P service must retain its charged resources without stopping other services on the connection. This transport chunk follows #942 and includes the service-session generalization from #956.
Solution
Negotiate complete persistent sessions from service declarations, with independent bounded queues, service-owned write deadlines, and shared admission resources held through cleanup. Record remote closes and write timeouts before a failed request claim can cancel the session, preserving download cooldowns and repeated-stall disconnects. Buffered responses wait for decode capacity and are validated before charging a stall. Remote closure cannot hide malformed frames. Local cancellation and body backpressure stay neutral, and connection shutdown interrupts pending validation. Dropping all application handles drains every member before retiring the session. Retained receivers and sender clones keep it alive even if incoming traffic arrives after an application receiver is dropped. The reader discards those frames after transport checks, preserving real peer-close detection. Reject duplicate offers while a remote session is active or retiring before reserving service capacity. If a retry races an abandoned incomplete session, discard both offers and use the existing cooldown so other services stay connected. Production block sync retains its current protocol until #945 activates the paired layout.
Testing
The malformed-frame backpressure regressions fail before the fix. At
596f91d78, all 371 selected block-sync, session, and transport tests pass without retries. Network all-target Clippy with warnings denied, formatting, Markdown lint, and changelog checks pass.The duplicate-offer regression fails before the fix at
35769d8bcand passes at253fa4e27. All 24 service-session tests pass. Atd6831f340, all 113 selected handler and block-sync session tests pass without retries, including duplicate rejection, capacity accounting, cleanup, reopening, and paired downloads. Network all-target Clippy with warnings denied, formatting, Markdown lint, and changelog checks pass. Prepared with Codex assistance.Changelog
Updated the transport fragment and service-session guide. Package versions follow the merged Iroh upgrade.